Onboard build failure analysis workflow - #132609

.github/workflows/build-failure-analysis.md:130

• mcp-servers.binlog-mcp.allowed: ["*"] grants the agent access to every tool the binlog-mcp MCP server exposes. Even with a digest-pinned container, this is a broader capability surface than necessary and makes future binlog-mcp tool additions automatically callable by the agent. Consider narrowing this allowlist to only the specific binlog_* tools the analyst is expected to use (and/or a supported wildcard that only matches binlog_*).

  binlog-mcp:
    container: "mcr.microsoft.com/dotnet-buildtools/prereqs:azurelinux-3.0-binlog-mcp-amd64"
    mounts:
      - "/tmp/binlogs:/data/binlogs:ro"
    allowed: ["*"]

.github/workflows/build-failure-analysis.md:519

• The per-artifact binlog staging counter i is only incremented on a successful cp. If one cp fails and the artifact contains multiple *.binlog files, the next file will reuse the same destination name (e.g. ${ai}_0_...) and can overwrite a previously staged binlog, contradicting the "destinations unique" comment and potentially hiding the real failing leg.

# on `needs.fetch-binlog.outputs.binlog-found == 'true'`, these only run once
# binlogs have been retrieved from the failed Azure DevOps build.
steps:
  - name: Download analysis artifact
    uses: actions/download-artifact@v8.0.1

.github/workflows/build-failure-analysis-command.md:460

• The per-artifact binlog staging counter i is only incremented when cp succeeds. If an artifact contains multiple *.binlog files and an early copy fails, subsequent files can reuse the same ${ai}_${i}_... destination name and overwrite staged data, which can make the analysis incomplete or misleading.

            # Info-ZIP prepends warnings on STDOUT for a recoverable archive,
            # and a multi-line value would still pass the `grep -qE` check
            # below, since `grep -q` matches if ANY line matches. `timeout`
            # bounds a hostile archive; pipefail + fail-closed because a killed
            # probe's partial output can end in a numeric column and undercount.

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build-failure-analysis.md:487

• The PIPESTATUS checks after unzip -Z1 | grep -qE ... can misclassify the "suspicious path" case as an unzip failure. When grep -q matches early it closes the pipe and unzip may exit non-zero due to SIGPIPE; checking zscan_rc[0] first will then hit the "could not list" branch instead of the intended "suspicious entry path" branch.

            if [ "${zscan_rc[0]}" -ne 0 ]; then
              echo "::warning::Skipping ${safe_name}: could not list archive entries (unzip -Z1 rc=${zscan_rc[0]})."; continue
            fi
            if [ "${zscan_rc[1]}" -eq 0 ]; then
              echo "::warning::Skipping ${safe_name}: archive has a suspicious (absolute or ..) entry path."; continue

.github/workflows/build-failure-analysis-command.md:541

• The PIPESTATUS checks after unzip -Z1 | grep -qE ... can misclassify the "suspicious path" case as an unzip failure. When grep -q matches early it closes the pipe and unzip may exit non-zero due to SIGPIPE; checking zscan_rc[0] first will then hit the "could not list" branch instead of the intended "suspicious entry path" branch.

            if [ "${zscan_rc[0]}" -ne 0 ]; then
              echo "::warning::Skipping ${safe_name}: could not list archive entries (unzip -Z1 rc=${zscan_rc[0]})."; continue
            fi
            if [ "${zscan_rc[1]}" -eq 0 ]; then
              echo "::warning::Skipping ${safe_name}: archive has a suspicious (absolute or ..) entry path."; continue

Previously missed (2) — in code that hasn't changed since the last review.

.github/workflows/build-failure-analysis.md:482

• The comment says anything left in /tmp/binlogs from an earlier run would otherwise be uploaded and attributed to this build, but the cleanup only removes *.binlog. Any other leftover files would still be uploaded with the artifact. Consider clearing the directory contents instead of only binlogs to match the stated invariant.

          # Only binlogs extracted by this run may be analyzed. Anything left in
          # the directory by an earlier run on the same runner would otherwise be
          # uploaded and attributed to this build.
          rm -f /tmp/binlogs/*.binlog

.github/workflows/build-failure-analysis-command.md:537

• The comment says anything left in /tmp/binlogs from an earlier run would otherwise be uploaded and attributed to this build, but the cleanup only removes *.binlog. Any other leftover files would still be uploaded with the artifact. Consider clearing the directory contents instead of only binlogs to match the stated invariant.

          # Only binlogs extracted by this run may be analyzed. Anything left in
          # the directory by an earlier run on the same runner would otherwise be
          # uploaded and attributed to this build.
          rm -f /tmp/binlogs/*.binlog

Suppressed comments (15)

Previously missed (4) — in code that hasn't changed since the last review.

.github/workflows/build-failure-analysis-command.md:471

• The job/artifact identity is made lossy before mapping: lowercasing, deleting all non-alphanumeric characters, and deduplicating collapses distinct timeline jobs such as Foo-Bar and Foo_Bar to foobar. The later artifact loop can then treat both artifacts as the same job, so if one is failed it may download the successful job's binlog too; preserve exact names and only use a normalized/prefix key when its identity is unique.
  .github/workflows/build-failure-analysis.md:407
• The job/artifact identity is made lossy before mapping: lowercasing, deleting all non-alphanumeric characters, and deduplicating collapses distinct timeline jobs such as Foo-Bar and Foo_Bar to foobar. The later artifact loop can then treat both artifacts as the same job, so if one is failed it may download the successful job's binlog too; preserve exact names and only use a normalized/prefix key when its identity is unique.
  .github/workflows/build-failure-analysis-command.md:154
• activation does not require binlog-found: the top-level if and generated lock gate on analysis-ready, while binlog-found only controls artifact upload/download. As written, this comment says the valid hlx-only path is disabled; update it to describe the actual gate.

This issue also appears on line 535 of the same file.
.github/workflows/build-failure-analysis.md:476

• The guard is now MAX_ZIP_BYTES=2147483648 (2 GB), but this explanation still says the per-artifact cap is 500 MB and that an ordinary build trips it. That is materially misleading for operators assessing disk/network exposure and for anyone deciding whether a skipped artifact should be expected; update the comment to describe the actual 2 GB limit and fallback behavior.

.github/workflows/build-failure-analysis-command.md:802

• This has the same failure mode as the automatic workflow: after analysis-ready=true, a transient artifact-service/upload failure fails fetch-binlog and suppresses the agent, rather than allowing the hlx-only path to run. Make the artifact handoff best-effort and let the agent proceed with no downloaded binlogs when the handoff is unavailable.

      - name: Upload analysis artifact

.github/workflows/build-failure-analysis-command.md:538

• The guard is now MAX_ZIP_BYTES=2147483648 (2 GB), but this explanation still says the per-artifact cap is 500 MB and that an ordinary build trips it. That is materially misleading for operators assessing disk/network exposure and for anyone deciding whether a skipped artifact should be expected; update the comment to describe the actual 2 GB limit and fallback behavior.

          # A 500 MB per-artifact cap is close enough to the size of a real
          # log artifact that an ordinary build trips it, and the job then
          # silently skips exactly the leg it exists to diagnose. Only one
          # archive is on disk at a time (each is deleted before the next

.github/workflows/build-failure-analysis-command.md:486

• The artifact-list request only consumes .value from one response. Azure DevOps paginates this endpoint via a continuation token, and this workflow explicitly expects roughly 150 Logs_Build_* artifacts; a failed leg on a later page will be missed and the agent may analyze incomplete evidence. Preserve and follow the continuation token (or otherwise prove the response is complete) before matching names.

          ado_get "artifact list" "${ADO_API}/build/builds/${BUILD_ID}/artifacts?api-version=7.1" || emit_none
          artifacts_json="${ADO_DOC}"
          mapfile -t all_names < <(printf '%s' "${artifacts_json}" | jq -r '.value // [] | map(select(.name | test("^Logs_Build_"))) | .[].name')

.github/workflows/build-failure-analysis-command.md:375

• This is another non-full-string numeric check: grep -q can accept the first all-digit line of a multi-line value. If the event/aw_context payload is malformed, PR_NUMBER can pass and then reach API paths and workflow-command logging. Use a Bash character-class check (or equivalent) that rejects any non-digit, including newlines.

          if ! printf '%s' "${PR_NUMBER}" | grep -qE '^[0-9]+$'; then

.github/workflows/build-failure-analysis-command.md:411

• This API-response validation has the same per-line grep -qE '^[0-9]+$' flaw: a malformed JSON field containing a numeric first line can pass, and BUILD_ID is then interpolated into later ::warning:: output and ADO URLs. Make the check reject every non-digit character rather than matching any line.

          if ! printf '%s' "${BUILD_ID}" | grep -qE '^[0-9]+$'; then

.github/workflows/build-failure-analysis-command.md:777

• The final gate only re-reads the PR head/merge SHA, so a newer runtime build can complete successfully for the same head while this command run is downloading/analyzing the older failed build; because this workflow sets cancel-in-progress: false, it will still post the stale failure analysis. Re-query the PR's latest runtime build and require the analyzed build to remain the newest failed build before allowing safe outputs, or otherwise invalidate this run when a newer build appears.

          LATEST_PR=$(gh api "repos/${GH_AW_REPO}/pulls/${PR_NUMBER}" 2>/dev/null)
          LATEST_HEAD=$(printf '%s' "${LATEST_PR}" | jq -r '.head.sha // empty')
          LATEST_MERGE=$(printf '%s' "${LATEST_PR}" | jq -r '.merge_commit_sha // empty')

.github/workflows/build-failure-analysis.md:739

• This upload is a hard step after the fetch script has emitted analysis-ready=true. A transient GitHub artifact-service/upload failure therefore marks fetch-binlog failed and prevents the downstream activation/agent jobs from running, so the documented hlx fallback is never reached even though binlogs are optional. Make the artifact handoff best-effort and have the agent continue with an empty binlog directory when upload/download fails.

        if: steps.fetch.outputs.binlog-found == 'true'

.github/workflows/build-failure-analysis.md:263

• This check is not actually full-string: grep applies ^...$ per line and -q succeeds if any line matches. A dispatch input such as 123\n456 therefore passes, after which BUILD_ID is used in ADO URLs and later ::warning:: output. Use a character-class check (or a null-delimited grep) that rejects every non-digit, including newlines.

          if ! printf '%s' "${BUILD_ID}" | grep -qE '^[0-9]+$'; then

.github/workflows/build-failure-analysis.md:320

• The same per-line grep -qE '^[0-9]+$' issue applies here: a malformed/free-form PR input containing a newline after a numeric line can pass validation, then be interpolated into GitHub API paths and ::warning:: messages. Replace this with a full-string character-class check that rejects newlines and all other non-digits.

          if ! printf '%s' "${PR_NUMBER}" | grep -qE '^[0-9]+$'; then

.github/workflows/build-failure-analysis.md:422

• The artifact-list request only consumes .value from one response. Azure DevOps paginates this endpoint via a continuation token, and this workflow explicitly expects roughly 150 Logs_Build_* artifacts; a failed leg on a later page will be missed and the agent may analyze incomplete evidence. Preserve and follow the continuation token (or otherwise prove the response is complete) before matching names.

          ado_get "artifact list" "${ADO_API}/build/builds/${BUILD_ID}/artifacts?api-version=7.1" || emit_none
          artifacts_json="${ADO_DOC}"
          mapfile -t all_names < <(printf '%s' "${artifacts_json}" | jq -r '.value // [] | map(select(.name | test("^Logs_Build_"))) | .[].name')

.github/workflows/shared/build-failure-analysis-shared.md:80

• The workflow declares the same required safe-outputs.data schema for both add_comment and create_pull_request_review_comment, but this instruction requires the structured object only for the summary. The agent's inline suggestion calls therefore have no required data payload and can be rejected by safe-output validation (or lose the artifact identity); require the object on every inline output as well.

   - Post exactly one summary via `add_comment` with structured data
     `{"workflow_artifact":"build-failure-analysis","artifact_kind":"analysis"}`
     and any inline
     `suggestion` blocks via `create_pull_request_review_comment`. Both

• Files reviewed: 5/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (9)

.github/workflows/build-failure-analysis-command.md:746

• The extraction loop reads find output as newline-delimited text. Archive entry names are PR-controlled and may contain newlines, so one *.binlog path can be split into multiple records; the fragments fail the [ -f ] check and are silently ignored, while any other files from the same artifact still make leg_staged > 0 and the leg is accepted. This contradicts the all-or-nothing comment and can omit evidence from a selected failed leg. Use NUL-delimited find -print0 with read -d '' (or otherwise reject malformed paths) so an unrepresentable entry fails the leg rather than being dropped.

            done < <(find "${AX_DIR}" -type f -name '*.binlog')

.github/workflows/build-failure-analysis-command.md:154

• This comment still says activation additionally requires binlog-found == 'true', but the workflow's top-level gate is analysis-ready == 'true' and the intended hlx-only path sets analysis-ready even when no binlog was found. The stale description can lead future edits to disable the no-binlog fallback; update it to say activation requires the verified failed build/revision, while binlog retrieval is optional.

    # and `activation` additionally requires `binlog-found == 'true'`.

.github/workflows/build-failure-analysis-command.md:645

• The cumulative compressed-download budget only adds the final size left in ZIP_TMP, but curl --retry 3 can download up to the cap on each failed attempt before truncating/replacing that file. Thus a nominal 3 GB total can still pull roughly four times that amount from Azure DevOps (and a hostile/unstable endpoint can consume the network/time budget without being charged). If this is intended as a network-egress ceiling, account for each attempt or disable retries/use a transfer mechanism with a true cumulative byte limit.

              timeout "${TIME_LEFT}" curl -sSL --fail --retry 3 --retry-delay 2 --connect-timeout 15 --max-time "${ATTEMPT_SECONDS}" --retry-max-time "${TIME_LEFT}" -o "${ZIP_TMP}" "${url}"
            ) 2>/dev/null
            curl_rc=$?
            ZIP_BYTES=$(stat -c%s "${ZIP_TMP}" 2>/dev/null || echo 0)
            # Charge the budget with the bytes retained on disk, including those of an
            # artifact about to be skipped. This is a disk and extraction budget, not a
            # meter of network egress: `-o` truncates before each retry, so failed
            # attempts are not counted here. What bounds those is FETCH_DEADLINE via
            # the `timeout` wrapper, plus `ulimit -f`, which caps every individual
            # attempt at ZIP_CAP.
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

.github/workflows/build-failure-analysis-command.md:514

• The artifact-to-job association is not actually exact here: when no exact normalized name matches, this accepts any artifact key that is a prefix of exactly one timeline job. For example, Logs_Build_NativeAOT would be mapped to the sole NativeAOT_Libraries job even though the artifact name does not identify that job. This can select a successful/non-corresponding artifact and contradicts the workflow's fail-closed “exact failed/canceled-job artifact” guarantee; use an authoritative artifact/job association or skip artifacts without an exact match.

              if [ -z "${mapped_job_key}" ]; then
                prefix_matches=0
                for job_key in "${all_job_keys[@]}"; do
                  if [[ "${job_key}" == "${artifact_key}"* ]]; then
                    mapped_job_key="${job_key}"
                    prefix_matches=$((prefix_matches + 1))
                  fi
                done
                [ "${prefix_matches}" -eq 1 ] || mapped_job_key=""

.github/workflows/build-failure-analysis-command.md:471

• The normalization here is lossy (tr -cd '[:alnum:]') and all_job_keys has already deduplicated the results. Distinct timeline jobs such as Foo-Bar and FooBar therefore collapse to one key; an artifact for the other job can be treated as an exact match and selected even when that job succeeded. Preserve the original job names (and reject normalized collisions) before using prefix/exact matching, otherwise the workflow can analyze the wrong leg instead of only failed/canceled jobs.

          mapfile -t failed_job_keys < <(
            printf '%s' "${timeline_json}" |
              jq -r '.records // [] | map(select(.type == "Job" and (.result == "failed" or .result == "canceled"))) | .[].name' |
              while IFS= read -r job_name; do
                printf '%s' "${job_name}" | tr '[:upper:]' '[:lower:]' | tr -cd '[:alnum:]'
                printf '\n'
              done |
              awk 'NF && !seen[$0]++'

.github/workflows/build-failure-analysis.md:682

• The extraction loop reads find output as newline-delimited text. Archive entry names are PR-controlled and may contain newlines, so one *.binlog path can be split into multiple records; the fragments fail the [ -f ] check and are silently ignored, while any other files from the same artifact still make leg_staged > 0 and the leg is accepted. This contradicts the all-or-nothing comment and can omit evidence from a selected failed leg. Use NUL-delimited find -print0 with read -d '' (or otherwise reject malformed paths) so an unrepresentable entry fails the leg rather than being dropped.

            done < <(find "${AX_DIR}" -type f -name '*.binlog')

.github/workflows/build-failure-analysis.md:450

• The artifact-to-job association is not actually exact here: when no exact normalized name matches, this accepts any artifact key that is a prefix of exactly one timeline job. For example, Logs_Build_NativeAOT would be mapped to the sole NativeAOT_Libraries job even though the artifact name does not identify that job. This can select a successful/non-corresponding artifact and contradicts the workflow's fail-closed “exact failed/canceled-job artifact” guarantee; use an authoritative artifact/job association or skip artifacts without an exact match.

              if [ -z "${mapped_job_key}" ]; then
                prefix_matches=0
                for job_key in "${all_job_keys[@]}"; do
                  if [[ "${job_key}" == "${artifact_key}"* ]]; then
                    mapped_job_key="${job_key}"
                    prefix_matches=$((prefix_matches + 1))
                  fi
                done
                [ "${prefix_matches}" -eq 1 ] || mapped_job_key=""

.github/workflows/build-failure-analysis.md:581

• The cumulative compressed-download budget only adds the final size left in ZIP_TMP, but curl --retry 3 can download up to the cap on each failed attempt before truncating/replacing that file. Thus a nominal 3 GB total can still pull roughly four times that amount from Azure DevOps (and a hostile/unstable endpoint can consume the network/time budget without being charged). If this is intended as a network-egress ceiling, account for each attempt or disable retries/use a transfer mechanism with a true cumulative byte limit.

              timeout "${TIME_LEFT}" curl -sSL --fail --retry 3 --retry-delay 2 --connect-timeout 15 --max-time "${ATTEMPT_SECONDS}" --retry-max-time "${TIME_LEFT}" -o "${ZIP_TMP}" "${url}"
            ) 2>/dev/null
            curl_rc=$?
            ZIP_BYTES=$(stat -c%s "${ZIP_TMP}" 2>/dev/null || echo 0)
            # Charge the budget with the bytes retained on disk, including those of an
            # artifact about to be skipped. This is a disk and extraction budget, not a
            # meter of network egress: `-o` truncates before each retry, so failed
            # attempts are not counted here. What bounds those is FETCH_DEADLINE via
            # the `timeout` wrapper, plus `ulimit -f`, which caps every individual
            # attempt at ZIP_CAP.
            TOTAL_ZIP_BYTES=$((TOTAL_ZIP_BYTES + ZIP_BYTES))

.github/workflows/build-failure-analysis.md:407

• The normalization here is lossy (tr -cd '[:alnum:]') and all_job_keys has already deduplicated the results. Distinct timeline jobs such as Foo-Bar and FooBar therefore collapse to one key; an artifact for the other job can be treated as an exact match and selected even when that job succeeded. Preserve the original job names (and reject normalized collisions) before using prefix/exact matching, otherwise the workflow can analyze the wrong leg instead of only failed/canceled jobs.

          mapfile -t failed_job_keys < <(
            printf '%s' "${timeline_json}" |
              jq -r '.records // [] | map(select(.type == "Job" and (.result == "failed" or .result == "canceled"))) | .[].name' |
              while IFS= read -r job_name; do
                printf '%s' "${job_name}" | tr '[:upper:]' '[:lower:]' | tr -cd '[:alnum:]'
                printf '\n'
              done |
              awk 'NF && !seen[$0]++'

• Files reviewed: 5/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

Suppressed comments (3)

.github/workflows/build-failure-analysis-command.md:613

• ulimit -f is configured in 1024-byte blocks, but rounding (ZIP_CAP + 1023) / 1024 up means the kernel permits a file larger than ZIP_CAP (by up to 1023 bytes). Because the post-download check only rejects >= ZIP_CAP, a no-Content-Length response can exceed both the per-artifact and remaining cumulative compressed-byte budgets. Round down (or add an exact-size enforcement loop) so the backstop cannot exceed the advertised cap.

[!NOTE] This review was created by GitHub Copilot.

              ulimit -f $(( (ZIP_CAP + 1023) / 1024 )) || exit 1

.github/workflows/build-failure-analysis.md:551

• ulimit -f is configured in 1024-byte blocks, but rounding (ZIP_CAP + 1023) / 1024 up means the kernel permits a file larger than ZIP_CAP (by up to 1023 bytes). Because the post-download check only rejects >= ZIP_CAP, a no-Content-Length response can exceed both the per-artifact and remaining cumulative compressed-byte budgets. Round down (or add an exact-size enforcement loop) so the backstop cannot exceed the advertised cap.

[!NOTE] This review was created by GitHub Copilot.

              ulimit -f $(( (ZIP_CAP + 1023) / 1024 )) || exit 1

.github/workflows/build-failure-analysis.md:468

• The new archive-handling path has no test that exercises the real curl/ulimit/unzip flow: the existing staging test mocks find, [ and cp, so it never verifies compressed-size accounting, uncompressed-size parsing, traversal rejection, extraction failure handling, or the cumulative/time budget exits. These guards are security- and reliability-critical and are duplicated in the command workflow; please add a fixture-driven test (or a focused script test) that runs the actual loop against small ZIPs and covers both accepted and rejected archives, including budget exhaustion.

          MAX_ZIP_BYTES=2147483648      # 2 GB compressed per artifact
          MAX_UNZIP_BYTES=2147483648    # 2 GB uncompressed per artifact
          MAX_TOTAL_BYTES=4294967296    # 4 GB uncompressed across all artifacts
          # Raising the per-artifact cap would otherwise raise the worst-case
          # number of bytes pulled over the network by the same factor, since
          # nothing else bounds the sum across artifacts. Cap the total
          # download too, and charge it *before* each transfer (see ZIP_CAP
          # below) rather than after, so the last artifact can't start just
          # under the limit and still pull a full MAX_ZIP_BYTES.

• Files reviewed: 7/9 changed files
• Comments generated: 0 new
• Review effort level: Lite